Skip to content

Conversation

@jycor
Copy link
Contributor

@jycor jycor commented Oct 25, 2024

Tests here: dolthub/dolt#8493

@jycor jycor marked this pull request as draft October 25, 2024 06:47
@jycor jycor marked this pull request as ready for review October 25, 2024 07:05
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me but it could use some more tests, very easy to get this stuff wrong. I see the test in Dolt but it would be nice to have one a Go test in GMS as well

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a mess. I agree that the same context that gets returned from BeginQuery should be used as the argument to ProcessList.EndQuery here and as the context argument to queryExec. But, I don't agree with a lot of details of this current function, including the code directly touched here:

  1. It seems like the err on line 422 should be checked.
  2. It seems like EndQuery should be called regardless of whether there was an error.
  3. Checking for ctx != nil in the defer block seems wrong.
  4. On line 414 we defer finalize(err), which is immediately evaluating the err variable, where as it seems like we meant to defer the evaluation until we know the error return of the actual function.
  5. Given that we want to defer evaluating the return value of this function it seems like we shouldn't shadow err on line 428. And, because we use a direct return of the callback evaluation on line 479, we either need to change that to err = callback... or we need to switch to named return values.

Made these cleanups here if you agree:

#2723

Otherwise looks good to me. Definitely an appropriate path to fixing the kill behavior we've seen. Good findings and investigation.

@jycor jycor merged commit a6973b5 into main Oct 28, 2024
8 checks passed
@jycor jycor deleted the james/ctx branch October 28, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants